Conversation
Codecov Report
@@ Coverage Diff @@
## master #12 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 79 92 +13
=====================================
+ Hits 79 92 +13
Continue to review full report at Codecov.
|
|
It looks like got them working, except something crazy happened on the node 6 test:
test/tap/index-test.js .............................. 17/18 5s |
|
@nomiddlename I got this to pass the tests. |
nomiddlename
left a comment
nomiddlename
left a comment
There was a problem hiding this comment.
Thanks for doing this, much appreciated. I've made a couple of comments, mostly regarding documentation - but there is a cut-and-paste layout from the tests in the main code that is a bit odd.
| ## Configuration | ||
|
|
||
| * `type` - `@log4js-ndoe/rabbitmq` | ||
| * `protocol` - `string` (optional, defaults to `amqp`) - the port the rabbitmq protocol option: `amqps` |
There was a problem hiding this comment.
Should this just read the rabbitmq protocol option?
| * `vhost` - `string` (optional, defaults to `/`) - vhost to use | ||
| * `layout` - `object` (optional, defaults to `messagePassThroughLayout`) - the layout to use for log events (see [layouts](layouts.md)). | ||
| * `shutdownTimeout` - `integer` (optional, defaults to `10000`) - maximum time in milliseconds to wait for messages to be sent during log4js shutdown. | ||
| * `frameMax` - `integer` (optional, defaults to `0`) - The size in bytes of the maximum frame allowed over the connection. 0 means no limit (but since frames have a size field which is an unsigned 32 bit integer, it’s perforce 2^32 - 1); I default it to 0x1000, i.e. 4kb, which is the allowed minimum, will fit many purposes, and not chug through Node.JS’s buffer pooling. [reference](https://www.squaremobius.net/amqp.node/channel_api.html) |
There was a problem hiding this comment.
Probably better to set the default to your 4kb value.
| const routingKey = config.routing_key || 'logstash'; | ||
| const vhost = config.vhost || '/'; | ||
| const heartbeat = config.heartbeat || 60; | ||
| const locale = config.locale || 'en_US'; |
There was a problem hiding this comment.
Should probably add locale to the docs as a configuration option.
| const locale = config.locale || 'en_US'; | ||
| const frameMax = config.frameMax || 0; | ||
| const keepAliveDelay = config.keepAliveDelay || 0; | ||
| const connectionTimeout = config.connection_timeout || 1000; |
There was a problem hiding this comment.
I don't think connectionTimeout is mentioned in the docs either.
| connection_timeout: connectionTimeout, | ||
| layout: { | ||
| type: 'pattern', | ||
| pattern: 'cheese %m' |
There was a problem hiding this comment.
Is this really the pattern you want to use?
| t.equal(result.fakeRabbitmq.state.msgs.length, 1, 'should be one message only'); | ||
| t.equal(result.fakeRabbitmq.state.msgs[0].toString(), 'cheese %m'); | ||
| t.end(); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
If you're talking to a fake RabbitMQ you shouldn't need a setTimeout, because it won't be doing anything asynchronous.
2 participants
added ssl support, stable retry for unstable socket connections (that may go down time-to-time).
keepalive is pretty important.